-
Notifications
You must be signed in to change notification settings - Fork 509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(document-extractor): split h4 sections #4609
Conversation
…rect heading links
It looks like within the
|
Looks like that was a hiccup. I reran all the actions and everything looks to be green now. |
@tannerdolby I cannot remember. With this PR, did you manage to address the issue that we ran into the last time we merged this work, or is that still on the to-do list? Thanks! |
@schalkneethling No problem! I've looked through the proposed changes here and couldn't pinpoint exactly what caused the issue when it was previously merged. If we can figure out how to create a reproducible example of that error then the problematic code should hopefully become more clear to us. |
@escattone Thoughts? |
@tannerdolby Thanks for all of your work to date on this! Unfortunately, the engineering team doesn't have much time to look into this further given other priorities. If you have the time and desire, for next steps I would recommend:
|
You're welcome @escattone! I have the time/desire to see this through so I'm happy to stick around until we pinpoint the root cause. I've done the steps you suggested and learned a few things. When running a full build on translated content within this branch, the first translated content page to raise an error is: /Users/TannerDolby/translated-content/files/de/learn/html/introduction_to_html/advanced_text_formatting/index.html
Error: MacroLiveSampleError within /Users/TannerDolby/translated-content/files/de/learn/html/introduction_to_html/advanced_text_formatting/index.html, line 145 column 4 (unable to find any live code samples for "Playable_code_1" within /de/docs/Learn/HTML/Introduction_to_HTML/Advanced_text_formatting) and for the remainder of the build many errors are logged but none of the pages actual fail and cause the build to halt. Inside of
As the only major errors when running a build on translated content are if (document.metadata.locale === "en-US") {
throw new Error(
`MacroLiveSampleError within ${flaw.filepath}, line ${flaw.line} column ${flaw.column} (${flaw.error.message})`
);
} else {
console.warn(
`MacroLiveSampleError within ${flaw.filepath}, line ${flaw.line} column ${flaw.column} (${flaw.error.message})`
);
} |
In this comment, Peter mentions, "So now, for en-US, it will crash the CI build, for translated-content it will just be a console.warn". Which is what I'm seeing, it will just be a console.warn (not an error that will crash the CI build) which I'm receiving in the logs, I just can't reproduce the CI build crashing. Since I didn't see any MacroLiveSampleErrors for There were other errors (aside from the MacroLiveSampleError) in the build for content and translated content like:
@schalkneethling I will use one of these first few translated pages as a test case and scan through my proposed changes in this PR to better figure out what could be causing problems for translated-content. I will do some further investigating. |
Thanks so much @tannerdolby. Much appreciated. |
@caugner Thanks for review so far. I've added the suggestions you had and everything is running now. The only problem is that functional testing is failing and Apologies for all the testing/debugging commits in this last batch. The
I haven't been in the codebase in awhile so I'm still getting familiarized with things again, but I could imagine the I had to add |
FWIW We need to make sure that doesn't break the EmbedLiveSample macro (see this issue). |
This pull request has merge conflicts that must be resolved before it can be merged. |
Summary
Fixes #4370
Problem
Currently only
<h1>-<h3>
are turned into direct links, which are incredible useful for sharing a link directly to a section in the docs. Since the h4 tags aren't apart of the section splitting logic, searching for a<h4>
which is deeply nested in a section can be time consuming if your trying to quickly share a direct reference to a section that begins with an h4.Solution
Add
<h4>
tags into the existing section splitting logic.Screenshots
Before
After
How did you test this change?
Running Yari locally and checking a few pages manually.
Original description
This branch is up-to-date with the latest
main
so the changes to the production workflow are included. I'm still in the process of running a full build of all the content to pinpoint where exactly the problematic code was.After running a full build (mdn/content and mdn/translated-content) using
yarn build
, it does successfully build the 42k pages in 74.5 minutes but with a bunch of flaws. Which the flaws might be fine, I'm just not sure what the baseline was in order to compare before/after.There were lots of
MacroLiveSampleError
saying "unable to find any live code samples for 'X'"but other than that nothing actually made the build break or fail. There were lots of CSSRef issues where the smartlink is broken
and this is the output from the full build (content and translated content)
cc @escattone